Conversation
… EFCore.SingleStore/Extensions
There was a problem hiding this comment.
Pull request overview
This pull request upgrades the SingleStore EF Core provider from EF Core 8 to EF Core 9. This is a major version upgrade that includes updating the .NET SDK to 9.0, updating all EF Core package dependencies to 9.0.0, and adapting to breaking changes in the EF Core 9 API surface.
Changes:
- Updated .NET SDK from 8.0 to 9.0 and target framework for tests from net8.0 to net9.0
- Updated EF Core dependencies from 8.0.13 to 9.0.0 and SingleStoreConnector from 1.2.0 to 1.3.0
- Converted synchronous test methods to async patterns throughout the test suite
- Adapted to EF Core 9 API changes including query compilation context, parameter processors, and expression handling
- Implemented new EF Core 9 features like migration database locking and precompiled query infrastructure
- Fixed various bugs in regex patterns, variable naming, and expression handling
- Added new test attributes and query translation methods
Reviewed changes
Copilot reviewed 148 out of 212 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| global.json, Version.props, Directory.Packages.props | Updated SDK version to 9.0.100, package versions to EF Core 9.0.0 |
| Multiple test files | Converted test fixtures to IAsyncLifetime, changed sync methods to async |
| Query/Internal/*.cs | Updated query compilation context and parameter processor signatures for EF Core 9 |
| Query/ExpressionVisitors/*.cs | Added bitwiseexpression visitor, updated expression handling |
| Migrations/Internal/*.cs | Implemented migration database locking, updated constructor signatures |
| Storage/Internal/*.cs | Fixed connection cloning to mark as owned |
| Infrastructure/*.cs | Added DefaultUtf8 collation properties, updated server version support |
| Extensions/*.cs | Added JsonOverlaps function, ComplexTypeProperty extensions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
src/EFCore.SingleStore.NTS/Query/Internal/SingleStoreMultiLineStringMemberTranslator.cs
Show resolved
Hide resolved
…SqlGeneratorTest and SingleStoreComplianceTest
| { | ||
| private const string MigrationsScript = nameof(MigrationsScript); | ||
|
|
||
| private const int LockTimeoutSeconds = 60 * 60 * 24 * 3; |
There was a problem hiding this comment.
is it it 3 days? Can you add a comment on why do we chose such a large timeout? Is it possible to make it configurable, if it makes sense?
| public override async Task<IMigrationsDatabaseLock> AcquireDatabaseLockAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| Dependencies.MigrationsLogger.AcquiringMigrationLock(); | ||
|
|
||
| var lockConnection = CreateLockConnection(); |
There was a problem hiding this comment.
CreateLockConnection is not async but is called from an async method, so there is an inconsistency here
| // We will therefore reverse that behavior to leaving the key order unchanged again. | ||
| // This will lead to two issues: | ||
| // - Migrations that upgrade vom Pomelo < 9.0 to Pomelo 9.0 will not include this change automatically, because the model | ||
| // never changed (we only made the change (before and now) here in MySqlMigrationsSqlGenerator). |
There was a problem hiding this comment.
probably need to update the comment as it mentions MySqlMigrationsSqlGenerator
|
|
||
| var typeMapping = mySqlCollateExpression.ValueExpression.TypeMapping ?? mySqlCollateExpression.TypeMapping; | ||
| var storeType = typeMapping?.StoreType?.ToLower(); | ||
| var varcharType = "VARCHAR(255)"; // Default |
There was a problem hiding this comment.
why is 255 default? Is this for the case when someone uses simply "VARCHAR" in the column definition?
| WHERE | ||
| `SCHEMA_NAME` = SCHEMA()"; | ||
| @@character_set_database AS `DEFAULT_CHARACTER_SET_NAME`, | ||
| @@collation_database AS `DEFAULT_COLLATION_NAME`;"; |
There was a problem hiding this comment.
https://docs.singlestore.com/db/v9.0/reference/configuration-reference/engine-variables/reserved-engine-variables/ these variables are no-op, maybe you need character_set_server and collation_server? If using no-op variables is intentional, please add a comment.
…MigrationLockTimeout configuration + explanations)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 982bae7. Configure here.
| var mySqlDbContextOptionsBuilder = new SingleStoreDbContextOptionsBuilder(optionsBuilder) | ||
| .TranslateParameterizedCollectionsToConstants(); | ||
|
|
||
| mySqlOptionsAction?.Invoke(mySqlDbContextOptionsBuilder); |
There was a problem hiding this comment.
User cannot override TranslateParameterizedCollectionsToConstants default
Medium Severity
TranslateParameterizedCollectionsToConstants is called before the user's mySqlOptionsAction callback, silently changing the EF Core default behavior. In all four UseSingleStore overloads, this forces constants mode even when users expect the EF Core 9 default (parameters). The test infrastructure itself has to explicitly work around this by calling TranslateParameterizedCollectionsToParameters() in SingleStoreTestStore.AddOptions. End users who don't know about this hidden override may get unexpected query behavior.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 982bae7. Configure here.
| [CanBeNull] this DbFunctions _, | ||
| DateTime startDate, | ||
| DateTime endDate) | ||
| => throw new InvalidOperationException(CoreStrings.FunctionOnClient(nameof(DateDiffNanosecond))); |
There was a problem hiding this comment.
New DateDiff functions overflow with int return type
Medium Severity
The new DateDiffNanosecond, DateDiffTick, and DateDiffMillisecond methods return int, but their SQL translations multiply or divide TIMESTAMPDIFF(MICROSECOND,...) results. DateDiffNanosecond multiplies by 1000, causing int overflow for time differences beyond ~2 seconds. DateDiffTick multiplies by 10, overflowing beyond ~3.5 minutes. The underlying TIMESTAMPDIFF returns BIGINT, but the translator casts to typeof(int).
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 982bae7. Configure here.
Note
Medium Risk
Upgrades core build/test infrastructure and dependency graph to EF Core 9/.NET 9, which can introduce subtle runtime/query translation differences. Also adds new provider options and LINQ function surface area that need validation against existing apps and migrations workflows.
Overview
Adds EF Core 9/.NET 9 support across the repo and CI. GitHub Actions now runs on PRs and
mainpushes, installs .NET9.0.101, and executes unit/integration/functional tests againstnet9.0.Moves to central package version management and bumps major dependencies. Removes
Dependencies.targets/Directory.Build.targets, introducesDirectory.Packages.props(EF Core[9.0.0,9.0.999],SingleStoreConnector1.3.0, updated xUnit/Roslyn/etc.), updatesglobal.json/dotnet-ef, and bumps package versioning to9.0.0.Extends provider capabilities and EF Core 9 compatibility. Adds
MigrationLockTimeoutoption, defaultsUseSingleStore(...)to callTranslateParameterizedCollectionsToConstants(), adds complex-typeHasCharSet()support (and scaffolding/codegen for it), expandsEF.Functionswith many newDateDiff*overloads plusJsonOverlaps, and makes small translation fixes (NTSCASENULL constant typing, MariaDB default UTF8 collations, suppresses EF9/CS1591 warnings).Reviewed by Cursor Bugbot for commit 982bae7. Bugbot is set up for automated code reviews on this repo. Configure here.